Add log spans using thread local storage in tests#4287
Add log spans using thread local storage in tests#4287joostjager wants to merge 3 commits intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
joostjager
force-pushed
the
std-spans
branch
2 times, most recently
from
38d439b to
5d2f39f
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4287 +/- ##
==========================================
- Coverage 86.61% 86.61% -0.01%
==========================================
Files 158 158
Lines 102730 102801 +71
Branches 102730 102801 +71
==========================================
+ Hits 88984 89045 +61
- Misses 11328 11338 +10
Partials 2418 2418
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
joostjager
force-pushed
the
std-spans
branch
3 times, most recently
from
1355c20 to
c3714c6
Compare
joostjager
force-pushed
the
std-spans
branch
2 times, most recently
from
14f583e to
aedfe0a
Compare
|
Rebased and fixed a The problem was that Fixed by making |
|
Taking this out of draft as I'm committed to adding span support, but putting it on hold while an alternative approach is being investigated: using proc-macros to transparently augment every method with an implicit logger parameter, which would allow passing the span stack through that invisible parameter instead of relying on TLS. |
joostjager
force-pushed
the
std-spans
branch
2 times, most recently
from
d84cd91 to
ee9b3bb
Compare
|
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
joostjager
force-pushed
the
std-spans
branch
2 times, most recently
from
aba1a33 to
e119381
Compare
Introduces LoggerScope, a RAII guard that pushes span names onto a thread-local stack. In test builds with std, log messages are prefixed with the current span chain (e.g., "[outer->inner]") for easier debugging. Includes: - LoggerScope struct with automatic cleanup on drop - #[log_scope] proc macro to annotate functions with a logging context - test_scope! macro for manual scope changes within long test functions Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
joostjager
changed the title
|
Updated PR to be test-only. |
|
🔔 2nd Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
Need to decide on what level the spans are most helpful, where to apply #[log_scope] in main. Aside from what devs may add ad-hoc during testing. |
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Might want to wait until we decide on #4322 since we'll want to re-use the proc-macro from there if we move forward. I guess they could move forward separately and just merge the macros on whichever lands second.
| /// [`ChannelCloseMinimum`]: crate::chain::chaininterface::ConfirmationTarget::ChannelCloseMinimum | ||
| /// [`NonAnchorChannelFee`]: crate::chain::chaininterface::ConfirmationTarget::NonAnchorChannelFee | ||
| /// [`SendShutdown`]: MessageSendEvent::SendShutdown | ||
| #[log_scope] |
There was a problem hiding this comment.
Can we just use a single proc macro that also scopes sub-functions? If this is for tests presumably that's more useful and also less clutter.
There was a problem hiding this comment.
Wasn't sure indeed if we want to manually decide what useful spans are, or we can simply auto-annotate all pub fns, and do a few more perhaps manually if they are called frequently in tests.
There was a problem hiding this comment.
Hmm, kinda up to you given you have the most experience with using it, I suppose. Definitely we should automate pub fns, but up to you if you want to automate all fns or make it explicit. ISTM I'm always trying to do a full trace including non-public fns, but if you have a different experience alright.
There was a problem hiding this comment.
I am a bit worried about presentation. Full trace might become a pretty long log line
3 participants
This is a stripped-down version of #4223 where only the spans are stored in thread local storage. This is a test-only change - the span functionality is compiled out in non-test builds, so there is no impact on production code or no-std users.
Example log output:
std-spans.log